Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

143 expand linexy constructors and slope information #150

Conversation

bbean23
Copy link
Collaborator

@bbean23 bbean23 commented Aug 8, 2024

No description provided.

@bbean23 bbean23 added the enhancement New feature or request label Aug 8, 2024
@bbean23 bbean23 self-assigned this Aug 8, 2024
@bbean23 bbean23 changed the base branch from main to develop August 8, 2024 14:36
braden6521
braden6521 previously approved these changes Aug 8, 2024
Copy link
Collaborator

@braden6521 braden6521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left one suggestion for extra documentation. Feel free to take or leave it.

Comment on lines 259 to 275
"""
Get a new instance of this class built from the rho + theta representation of a line.

Parameters
----------
rho : float
The right angle distance between the line and the origin (0,0). For
images, this will be the top-left corner of the image.
theta : float
The angle of the line, in radians, for the standard graphing
coordinate system. 0 is on the positive x-axis to the right, and the
angle increases counter-clockwise.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example here or clarify. For example, if you had rho=1 and theta=0, would the line pass through x=+1 or x=-1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you left a comment. I double-checked the method, and realized I had both a bug and a bad definition.

  • Changed the definition of theta from "angle of the line" to "angle between the x-axis and the right angle distance vector".
  • Removed the cast to an integer, which was causing numeric issues.
  • Added unit tests for from_rho_theta() and from_location_angle().

Copy link
Collaborator

@braden6521 braden6521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@bbean23 bbean23 force-pushed the 143-expand-linexy-constructors-and-slope-information branch from fd22626 to 2c42308 Compare August 10, 2024 00:07
@bbean23 bbean23 merged commit 61451fe into sandialabs:develop Aug 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants